-
-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Brewing recipes #161
base: 1.19.4
Are you sure you want to change the base?
Brewing recipes #161
Conversation
This will break with vanilla clients |
How does this break vanilla? And is it any better ways? |
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/impl/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
If a datapack (or otherwise server side mod) uses that feature, vanilla clients won't be able to join server, as recipe packet will contain recipe type it can't read. QSL and it's datapackable additions should be compatible with vanilla clients |
As the recipe is only used serverside, if we can strip the recipes from being sent to a vanilla client this would work. But... that sounds very hacky. I'm not familiar at all with Minecraft or Quilt's networking so I don't know if it's possible to detect vanilla clients and strip the recipes if so. We could just strip them entirely from being sent to clients, but I wouldn't want to do that for recipe integrations in mods like EMI. |
It should be possible. Polymer library does that for example https://github.com/Patbox/polymer/blob/dev/1.19.1/polymer/src/main/java/eu/pb4/polymer/mixin/item/packet/SynchronizeRecipesS2CPacketMixin.java#L37 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that the builders extend an impl class, that's not possible.
library/data/recipe/src/main/resources/data/quilt/tags/items/potions.json
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/impl/RecipeImpl.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/impl/RecipeImpl.java
Outdated
Show resolved
Hide resolved
...data/recipe/src/main/java/org/quiltmc/qsl/recipe/mixin/SynchronizeRecipesS2CPacketMixin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far, just some Javadoc nitpicks...
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
Co-authored-by: ADudeCalledLeo <[email protected]>
Co-authored-by: ADudeCalledLeo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some package level docs, especially for the different recipe types and their differences would be nice. The rest of the API looks great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good! There is just one spot that I feel like it could be improved, however, i'm not sure if it's possible, i might wanna do my own tests
...ary/data/recipe/src/main/java/org/quiltmc/qsl/recipe/mixin/BrewingStandBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with this part of Minecraft, so a lot of these questions come from ignorance. Sorry!
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/PotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
# Conflicts: # library/data/recipe/build.gradle # library/data/recipe/src/main/resources/quilt_recipe.mixins.json # library/item/item_content_registry/src/main/java/org/quiltmc/qsl/item/content/registry/impl/ItemContentRegistriesClientInitializer.java # library/item/item_content_registry/src/main/java/org/quiltmc/qsl/item/content/registry/impl/ItemContentRegistriesInitializer.java # library/item/item_content_registry/src/testmod/resources/data/quilt/attachments/minecraft/item/brewing_fuel_time.json
It looks like it's time for a re-reviewing of this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh dear, i was expecting to bikeshed the API itself a lot, but nope! i only have style complaints (with only one concern about registry stuff);
I am pretty satisfied with the API itself, so it gets an Ennui's Stamp of Approval
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/AbstractBrewingRecipe.java
Outdated
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
.../data/recipe/src/main/java/org/quiltmc/qsl/recipe/api/brewing/CustomPotionBrewingRecipe.java
Outdated
Show resolved
Hide resolved
library/data/recipe/src/main/java/org/quiltmc/qsl/recipe/impl/RecipeImpl.java
Outdated
Show resolved
Hide resolved
...ary/data/recipe/src/main/java/org/quiltmc/qsl/recipe/mixin/BrewingStandBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
..._registry/src/main/java/org/quiltmc/qsl/item/content/registry/api/ItemContentRegistries.java
Outdated
Show resolved
Hide resolved
...n/java/org/quiltmc/qsl/item/setting/mixin/recipe_remainder/BrewingStandBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
...tem_setting/src/testmod/java/org/quiltmc/qsl/item/test/mixin/BrewingRecipeRegistryMixin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get this into FCP soon
|
||
import org.quiltmc.qsl.recipe.impl.RecipeImpl; | ||
|
||
// TODO Conditionally remove only for vanilla clients |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really need to be considered ASAP because of mods like EMI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #298 is something to keep an eye into, since it might help with that
4 months later... |
Allows the creation of brewing recipes in a similar fashion to other recipe types.
The three kinds of brewing recipes are
quilt:potion_brewing
,quilt:custom_potion_brewing
, andquilt:potion_item_brewing
. potion brewing and custom potion brewing are functionally similar, with the addition that custom potion brewing allows multiple status effects, much like the turtle master potion but without having to register a potion first. potion item brewing is how you transform a regular potion into a splash potion, etc.This PR also de-hardcodes the potion slots in the brewing stand, having it check a tag,
quilt:potions
instead.The base
AbstractBrewingRecipe
and supporting infrastructure are general enough that they should support a subclass providing recipes on arbitrary item stacks.Additionally, this PR opens up a brewing stand fuel registry as an item content registry to allow modders and users to easily add new fuels or modify existing ones.